gh-145742: Pre-tag operands for _LOAD_CONST_INLINE_BORROW variants#148081
gh-145742: Pre-tag operands for _LOAD_CONST_INLINE_BORROW variants#148081corona10 wants to merge 24 commits intopython:mainfrom
Conversation
|
@Fidget-Spinner @markshannon PTAL when you have times :) |
|
I’ve just resolved all the conflicts. Gentle ping to @markshannon and @Fidget-Spinner. |
|
Thanks for doing this. There is work underway to remove the many variants of |
|
Got it |
|
@markshannon I've resolved conflicts with the latest main and simplified the pre-tagging/untagging to be path-based (auto-tagging in add_op() when the opcode is a BORROW variant). PTAL. |
markshannon
left a comment
There was a problem hiding this comment.
Have you tested this with Py_STACKREF_DEBUG set?
| #define PyStackRef_FromPreTagged(ptr) _PyStackRef_FromPreTagged((uintptr_t)(ptr)) | ||
|
|
||
| /* Tag a PyObject pointer as a borrowed operand for BORROW variants. */ | ||
| #define PyStackRef_TagBorrow(ptr) \ |
There was a problem hiding this comment.
Doesn't it need defining for Py_STACKREF_DEBUG as well?
Can you make this an inline function, for type safety?
| PyObject *ptr = (PyObject *)CURRENT_OPERAND0_64(); | ||
| value = PyStackRef_FromPyObjectBorrow(ptr); | ||
| _PyFrame_SetStackPointer(frame, stack_pointer); | ||
| value = PyStackRef_FromPreTagged(ptr); |
There was a problem hiding this comment.
You need to whitelist PyStackRef_FromPreTagged as non-escaping
FYI, it's already broken see: #148718 |
|
@markshannon gentle ping? |
|
This looks fine, once updated to main. The problem I have with it is the lying to the type system and tagged pointers. So, can you either:
|
|
|
||
| tier2 pure op(_LOAD_CONST_INLINE_BORROW, (ptr/4 -- value)) { | ||
| value = PyStackRef_FromPyObjectBorrow(ptr); | ||
| value = PyStackRef_FromPreTagged(ptr); |
There was a problem hiding this comment.
The problem is that we always cast /4 values to PyObject * even when they aren't pointers.
|
When you're done making the requested changes, leave the comment: |
I prefer this one, since this PR is not urgent thing :) |
|
@markshannon I’d like to know whether this is the kind of DSL you had in mind, or if you would prefer a different grammar. I would be happy to follow your suggestion if you have one. PTAL. op ( LOAD_CONST_INLINE_BORROW, (ptr/4^ -- value) ) {
value = PyStackRef_FromPreTagged(ptr);
} |
|
FYI, the current Ubuntu CI failure is not related to this change; it's a Canonical infra issue that they are handling at this moment. https://x.com/ubuntu/status/2050112955132297652 |
|
DSL Summary
|
Uh oh!
There was an error while loading. Please reload this page.